Use rapids-logger to generate the cuml logger #6162
Conversation
vyasr
added
improvement
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
1 similar comment
|
/ok to test |
…till not fully compiling due to missing logger functions)
|
/ok to test |
|
/ok to test |
|
/ok to test |
| @@ -434,7 +435,18 @@ class UMAP(UniversalBase, | |||
| self.precomputed_knn = extract_knn_infos(precomputed_knn, | |||
| n_neighbors) | |||
|
|
|||
| logger.set_level(verbose) | |||
There was a problem hiding this comment.
This logic was previously incorrect because it was propagating boolean verbosity values directly without accounting for the boolean handling done in Base's constructor.
| @@ -44,7 +44,7 @@ class UMAP(BaseEstimator, DelayedTransformMixin): | |||
| >>> X, y = make_blobs(1000, 10, centers=42, cluster_std=0.1, | |||
| ... dtype=np.float32, random_state=10) | |||
|
|
|||
| >>> local_model = UMAP(random_state=10) | |||
| >>> local_model = UMAP(random_state=10, verbose=0) | |||
There was a problem hiding this comment.
We now must set this because the default logging level will result in an info log that was previously being incorrectly suppressed.
|
The current failure should be fixed by rapidsai/rmm#1774. We're running into problems when building wheels because clones of rmm bring in the dated version of rapids-logger that was originally in that repository. |
| CPMAddPackage( | ||
| NAME rapids_logger GITHUB_REPOSITORY rapidsai/rapids-logger GIT_SHALLOW FALSE GIT_TAG | ||
| 4df3ee70c6746fd1b6c0dc14209dae2e2d4378c6 VERSION 4df3ee70c6746fd1b6c0dc14209dae2e2d4378c6 | ||
| ) | ||
| rapids_make_logger( | ||
| ML EXPORT_SET cuml-exports LOGGER_HEADER_DIR include/cuml/common/ LOGGER_MACRO_PREFIX CUML LOGGER_TARGET cuml_logger | ||
| ) |
There was a problem hiding this comment.
I presume this is something that happens in all repos that use rapids-logger, I wonder if a function in rapids-cmake would be a good idea instead of using CPMAddPackage directly?
There was a problem hiding this comment.
Yes absolutely. Every repository will have to do its own call to rapids_make_logger to provide the right arguments, but I plan to replace the CPMAddPackage call with a rapids-cmake call. I will do that a bit later though once I'm ready to synchronize all the repos because right now they are using different commit hashes and I'll be adding a couple more features to the trunk of rapids-logger before I synchronize all the repos using it.
There was a problem hiding this comment.
Starting to address this in rapidsai/rapids-cmake#737.
| @@ -195,10 +138,16 @@ def set_pattern(pattern): | |||
| pattern for a code section, as described in the example section above. | |||
| """ | |||
| IF GPUBUILD == 1: | |||
| cdef string prev = Logger.get().getPattern() | |||
| context_object = PatternSetter(prev.decode("UTF-8")) | |||
| # TODO: We probably can't implement this exact API because you can't | |||
There was a problem hiding this comment.
Can you create a github issue to track this?
|
/merge |
This PR replaces cuml's logger implementation with one generated using https://github.com/rapidsai/rapids-logger. This approach allows us to centralize the logger definition across different RAPIDS projects while allowing each project to vendor its own copy with a suitable set of macros and default logger objects. The common logger also takes care of handling the more complex packaging problems around ensuring that we fully isolate our spdlog dependency and do not leak any of its symbols, allowing our libraries to be safely installed in a much broader set of environments.
This PR requires rapidsai/rapids-logger#1
Contributes to rapidsai/build-planning#104